fix: make data property stricter
#327
Conversation
|
It seems that the CI failure is unrelated to this change. |
|
I investigated this and it appears to be a regression in I tried addressing it but couldn't find a clear solution, so I opened an issue in the |
lumirlumir
force-pushed
the
fix-make-the-data-property-stricter
branch
from
1789094 to
440b8c2
Compare
|
Marking it as a draft until #332 is merged. |
|
This PR is now ready for re-review. |
There was a problem hiding this comment.
Pull request overview
This PR restricts the data property type in violation reports and suggestions from the overly permissive Record<string, unknown> to a new stricter type MessagePlaceholderData that only allows primitive types: string | number | boolean | bigint | null | undefined. This prevents non-serializable types (objects, arrays, functions, symbols, etc.) from being used in message placeholders, which is important since these values are typically converted to strings for display in error messages.
- Introduced
MessagePlaceholderDatatype to enforce strict typing on message placeholder data - Updated both
ViolationReportBaseandSuggestedEditBaseinterfaces to use the new type - Added comprehensive type tests to verify allowed primitives work correctly and disallowed types (objects, arrays, functions, symbols, maps, sets) are properly rejected at compile time
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/types.ts | Defines new MessagePlaceholderData type and updates data property types in ViolationReportBase and SuggestedEditBase interfaces |
| packages/core/tests/types/types.test.ts | Adds comprehensive test coverage for allowed primitive types and uses @ts-expect-error to verify disallowed complex types are properly rejected |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…com/eslint/rewrite into fix-make-the-data-property-stricter
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
LGTM. Would like another review before merging.
github-project-automation
bot
moved this from Second Review Needed
to Complete
in Triage
Prerequisites checklist
What is the purpose of this pull request?
In this PR, I've resolved the issue metioned in #310.
I've updated the
dataproperty to acceptstring | number | boolean | bigint | null | undefinedand added test cases to ensure it works.What changes did you make? (Give an overview)
In this PR, I've resolved the issue metioned in #310.
Related Issues
Closes: #310
Is there anything you'd like reviewers to focus on?
N/A